This is memoized to stop the directInsertValue array from getting recreated on every render, and causing an infinite loop because it changes innerBlocks, which in turn the Navigation function depends on to calculate the directInsertValue 😖

I can't think of a better way to do it though. I think this logic should reside in the block, because it might vary a lot - Submenus are set to always use it (at least until we sort out mega menus), and conceivably there could be blocks using it depending on their nesting status or some other condition.

There are existing mechanisms in variations and blocks to define whether they can be used in the inserter. Introducing some level of granularity there might be an alternative to this prop.

e.g.

"supports": {
    "inserter": "__experimentalQuick" | "__experimentalGlobal" | true | false
}

I remember @gwwar also looked into a default block for the navigation block, so going back to that might also be an option.

The related PR was here, where we explored adding a default block per container (vs one for the entire editor). #29490.

If I recall we ran into some of the same issues, where we made a guess as to what folks wanted to insert, but transforming blocks after that was more difficult.

Let's highlight the default block better in the quick inserter.

🤔 From #34041 it might be worth trying an __experimentalDefaultBlock block list setting again.

I'll try tidying that experimental branch to see if its viable.

There are existing mechanisms in variations and blocks to define whether they can be used in the inserter. Introducing some level of granularity there might be an alternative to this prop.

I don't think a setting in the block.json will work in this case, because we need to change it based on the block's innerBlocks. If any of the innerBlocks are not Nav Links, the quick inserter should appear instead of directly inserting a Link.

For the same reason, I'm not sure we'd want to leverage @gwwar 's __experimentalDefaultBlock, because the Insert before and Insert after buttons would be tied to its value. If we use __experimentalDefaultBlock, as it stands in #29490, to tell the block inserter when we want a default block to be inserted directly, we'll end up disabling Insert before and Insert after whenever the Navigation contains anything other than Links, which is quite unexpected.

Independently of whether we do enable Insert before and Insert after on all block containers in #29490 (which I think we should! it makes total sense) we still want to make it easier to insert Nav Links inside the Nav block, and a workflow that forces us to look for a deeply hidden option in the block toolbar doesn't satisfy that requirement.

In order to keep useInnerBlocksProps simple it would be nice if we could leverage the same prop for both use cases, but I'm not sure it will be possible. Am keen to know if anyone has other ideas around this though!

That part of it is pretty complicated. There's still this part of the issue that indicates a way to designate a default block within the nav block will be needed:

Let's highlight the default block better in the quick inserter. It should always come in first. A user should not have to think when they open a quick inserter if they are not looking for a specific block.

I don't know that the current directInsert prop will work well with that.

Maybe an option is to look at using renderAppender to build out a lot of the custom insertion behavior first before formalising it into props and configuration.

I'm not sure if __experimentalDefaultBlock is viable, but the idea of a default block other than a global one for the editor may still be useful. (The buttons block inserter cheats a bit since only one child item is allowed and doesn't apply well to other situations).

I mostly just rebased the branch yesterday and resolved conflicts. I'll ping y'all if I think there might be viable flow to look at.

Maybe an option is to look at using renderAppender to build out a lot of the custom insertion behavior first before formalising it into props and configuration.

Not sure if I'm missing something, but renderAppender seems to be used only for determining what the appender looks like. It doesn't feel right to mix that with its behaviour (whether it should display quick inserter or insert a block directly), because we'll want to be able to set both independently.

There's still this part of the issue that indicates a way to designate a default block within the nav block will be needed

This is a good point. I'm thinking we could have two props: a defaultBlock array that specifies the default block name and any relevant attributes, and a directInsert boolean that specifies whether the default block, if it exists, should be inserted directly or not. That way defaultBlock can be used to specify the default for the quick inserter, as well as for insert before and after. If I'm not mistaken defaultBlock will need to be passed to useInnerBlockProps for all of these scenarios.

@gwwar I'm going to steal the__experimentalDefaultBlock prop and give it a go in this PR 😂

Go for it ✨

I think the default block can work as an array or object. It may help to give a few more hints on the data structure with something like:

const [ name, attributes, innerBlocks ] = defaultBlock;
createBlock( name, attributes, innerBlocks );

Can you think of a scenario where we'd need defaultBlock to be an object? In the existing use cases (both this and your other PR) we're using it to store values to pass to createBlock so it makes more sense as an array.

In this PR, it's defined as an array in the jsdocs for the parameters passed to useNestedSettingsUpdate and the return value of __experimentalGetDirectInsertBlock. We'd need to change those if we wanted to support an object too.

For me, a good reason to make it an object would be passing around a block instead of an array of arguments for createBlock. For example, this API could look like const DEFAULT_BLOCK = createBlock( 'core/navigation-link' ). One thing I don't like there is creating a uuid and a few other things we don't really need.

Maybe that could be a function then? For example const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-link' )

🤔 Good point, I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks. A function would make sense in that case, though we might want to pick a friendlier name. (I find that some folks get confused by the creator name). Maybe INSERT_DEFAULT_BLOCK or something similar?

Edit: Maybe a simple name like getDefaultBlock? One other piece of this is we'll likely need to modify the suggested items in the quick inserter as part of 34041. (The quick inserter is currently ordered by block frecency).

Hmm, I'm not so sure about this. When allowedBlocks is in use, the block is created directly in the inserter. I'd prefer to emulate that logic: fetching a block type, and creating the block from that type in the inserter. Default block and allowed block shouldn't be too different, because they essentially serve the same purpose. The main difference is that only one default block may exist even if multiple blocks are allowed.

I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks

I'm not sure I follow. We shouldn't have any reason to pass innerBlocks to the inserter, because it should always insert a brand new block.

I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks

I think @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:

const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-menu', {}, [
    createBlock( 'core/navigation-link', { title: "Home" }, [ ] ),
    createBlock( 'core/navigation-link', { title: "About" }, [ ] ),
] )

But as you say, I don't think it's a use-case that exists today.

Default block and allowed block shouldn't be too different, because they essentially serve the same purpose.

Hm I just thought of a completely different perspective here. Could we just make it a string to point out which supported block do we want to insert by default? It must be one of the supported blocks and we don't specify any attributes at the moment so maybe we don't need anything more? Or are we running in circles now?

I think @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:

That's right. A better example might be a container block that makes little sense without children, for example the buttons block.

Since we don't have an explicit case for passing though attributes or innerBlocks yet, I think it's fine deferring needing to pass through extra arguments until we need them. Whichever combination you prefer @tellthemachines

const blockToInsert = directInsertBlock?.length
					? createBlock( ...directInsertBlock )
					: createBlock( allowedBlockType.name )

One thing to note is that if folks pass through innerBlocks as the third item in an array, it's possible to have innerBlocks with static uuids. It's an edge case, but two options I can think of:

1. clone the block

insertBlock( cloneBlock( blockToInsert ), getInsertionIndex(), rootClientId );

2. Avoid passing through innerBlocks if we don't want to handle this case yet

const [ name, attributes ] = directInsertBlock;
createBlock( name, attributes )

Have we considered allowing __experimentalDirectInsert to be a match function which returns true or false? It doesn't look like we need any other information specific to the render function. One benefit is that we can then make it a constant and can define it next to the default block.

For example it might look like:

const DIRECT_INSERT = ( block ) => { 
    return block.innerBlocks.every( ( { name } ) => name === 'core/navigation-link' || name === 'core/navigation-submenu' );
};

And in the selector we can update it to call the match function.

Oooh nice! I'll try it out.

Hmm, this doesn't work well when we either add or remove other block types, because the selector won't re-run with every block update, so it won't know when the content has changed. Whereas if we keep the logic inside the edit function it updates with every change in the inner blocks.

Have you tried without createSelector? We should get proper results provided we recalculate whenever the block.innerBlocks reference updates.

Edit: I see some earlier comments about an infinite loop. I'll 🔍 a bit. This is more of a nice to have if we can define this outside.

The following works for me. (I didn't notice any infinite loops, but let me know if there's a specific case to test.)

Non-memo'd:

export const __experimentalGetDirectInsertBlock = (
	state,
	rootClientId = null
) => {
	if ( ! rootClientId ) {
		return;
	}
	const settings = state.blockListSettings[ rootClientId ];
	const defaultBlock = settings?.__experimentalDefaultBlock;
	if ( ! defaultBlock ) {
		return;
	}
	const directInsert = settings?.__experimentalDirectInsert(
		getBlock( state, rootClientId )
	);
	if ( ! directInsert ) {
		return;
	}

	return defaultBlock;
};

Or memo'd

export const __experimentalGetDirectInsertBlock = createSelector(
	( state, rootClientId = null ) => {
		if ( ! rootClientId ) {
			return;
		}
		const settings = state.blockListSettings[ rootClientId ];
		const defaultBlock = settings?.__experimentalDefaultBlock;
		if ( ! defaultBlock ) {
			return;
		}
		const directInsert = settings?.__experimentalDirectInsert(
			getBlock( state, rootClientId )
		);
		if ( ! directInsert ) {
			return;
		}

		return defaultBlock;
	},
	( state, rootClientId ) => [
		state.blockListSettings[ rootClientId ],
		state.blocks.tree[ rootClientId ],
	]
);

Oh I see, I was forgetting to add state.blocks.tree[ rootClientId ] to the dependencies 🤦

I think we should also accept a boolean value though, to avoid having to do something like () => true when we want to always insert the default block.

I think we should also accept a boolean value though

That sounds reasonable to me 👍